Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove metadata casts to byte #3706

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

RecursivePineapple
Copy link
Contributor

@RecursivePineapple RecursivePineapple commented Dec 24, 2024

These are incompatible with NEID and cause problems with blocks whose metadata can go over 127.

This shouldn't have any side effects since nothing changed here relies on bit math.

I ran a quick sanity test on everything I touched to make sure I didn't break anything.

I didn't change some stuff related to ores & prospecting because I didn't want to create merge conflicts for my ore refactor PR, which already has those changes.

I'm sure I've missed something, but this should be most of the casts & byte params/returns. I used this regex to find these: byte.*meta (case insensitive), then I fixed the syntax errors that resulted from the changes.

These are incompatible with NEID and can cause problems with blocks
whose metadata can go over 127.
@RecursivePineapple RecursivePineapple added refactor For PRs rewritting a part of the code to have a nicer code overall. bug fix Fix a bug. Please link it in the PR. labels Dec 24, 2024
@RecursivePineapple RecursivePineapple requested a review from a team December 24, 2024 20:47
@serenibyss
Copy link
Member

@RecursivePineapple Did you test this in full pack? There are probably changes needed in some other mods that add machines

@RecursivePineapple
Copy link
Contributor Author

@serenibyss no I didn't, thanks for reminding me

@RecursivePineapple
Copy link
Contributor Author

Hey @serenibyss, I tested this in the full pack and it didn't crash. I tried searching for any non-gt5u deps that would be broken by this but I couldn't find any, so I'm not sure what items or blocks I should test. I figure that it'd crash if there were any method signature incompatibilities.

@Dream-Master
Copy link
Member

is this zeta safe to test ? @RecursivePineapple

@RecursivePineapple
Copy link
Contributor Author

@Dream-Master I didn't run it on a gate base because I couldn't find a link for it, but I did test it in an SP world. It should be relatively low-risk since any linkage errors should show up during modpack start, but I'm not sure what the standards for zeta are.

@Dream-Master
Copy link
Member

ok will add next update

@serenibyss serenibyss added the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label Dec 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Fix a bug. Please link it in the PR. 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta refactor For PRs rewritting a part of the code to have a nicer code overall.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants